Use scrollTop instead of getBoundingClientRect to control VirtualList rendering#4607
Merged
Conversation
canova
approved these changes
May 17, 2023
canova
left a comment
canova
left a comment
Member
There was a problem hiding this comment.
Looks good to me with the change in event type name I mentioned below. Thanks!
… rendering This works around bug-1831148 (https://bugzil.la/1831148).
…ange event listener
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This patch does more things than what I initially wanted:
commit 1: minor refactoring in our ResizeObserver wrapper, that makes the commit 2 easier to follow. It's also more correct, because I believe that the current version would stop working if we unregistered all customers and register some new ones without creating the wrapper again.
commit 2: move the
visibilitychangeevent handling from WithSize to the ResizeObserver wrapper, so that this behavior is present even when we don't useWithSize. I wonder if WithSize is even needed after this, we could just use the ResizeObserver wrapper directly now...commit 3: this is the initial goal for this patch:
scrollTopis used instead ofgetBoundingClientRect.commit 4: this uses the ResizeObserver wrapper to capture the content height. This fixes another (not very visible) issue:
=> at one point, the panel is blank because it's not rerendered when the window's size changes.
production (notice that the marker table is blank below approximately half the height)
deploy preview